Skip to content

[libcu++] Fix Overflow in Dynamic Shared Memory Validation#9727

Open
davebayer wants to merge 1 commit into
NVIDIA:mainfrom
davebayer:fix_overflow_dynamic_smem
Open

[libcu++] Fix Overflow in Dynamic Shared Memory Validation#9727
davebayer wants to merge 1 commit into
NVIDIA:mainfrom
davebayer:fix_overflow_dynamic_smem

Conversation

@davebayer

@davebayer davebayer commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@davebayer davebayer requested a review from a team as a code owner July 7, 2026 10:58
@davebayer davebayer requested a review from Jacobfaib July 7, 2026 10:58
@github-project-automation github-project-automation Bot moved this to Todo in CCCL Jul 7, 2026
@cccl-authenticator-app cccl-authenticator-app Bot moved this from Todo to In Review in CCCL Jul 7, 2026
@coderabbitai

coderabbitai Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 14bc6319-f0d0-4e05-b4d6-a3002e43ed70

📥 Commits

Reviewing files that changed from the base of the PR and between a6109c1 and 3c29a94.

📒 Files selected for processing (1)
  • libcudacxx/include/cuda/__launch/configuration.h
🚧 Files skipped from review as they are similar to previous changes (1)
  • libcudacxx/include/cuda/__launch/configuration.h
📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Improved validation for dynamic shared memory requests that use unbounded arrays.
    • Added safer size computation to detect multiplication overflow and oversize byte totals, preventing incorrect calculations.
    • Now returns a clearer invalid_argument error when the requested dynamic shared memory exceeds the portable limit or cannot be represented safely.
    • Kept public interfaces unchanged while making the underlying check more reliable.

Walkthrough

dynamic_shared_memory(_Tp)(__n) now uses cuda::mul_overflow to compute requested bytes and rejects overflowed or over-limit sizes with std::invalid_argument. The header also includes cuda/__numeric/mul_overflow.h.

Changes

Dynamic Shared Memory Overflow Check

Layer / File(s) Summary
Overflow-safe size computation
libcudacxx/include/cuda/__launch/configuration.h		 Adds <cuda/__numeric/mul_overflow.h> and changes the unbounded dynamic_shared_memory(__n) check to use mul_overflow(__n, sizeof(value_type)) before comparing against the portable dynamic shared memory limit.

Related PRs: None identified.

Suggested labels: bug, libcudacxx

Suggested reviewers: None identified.

Comment @coderabbitai help to get the list of available commands.

@github-actions

This comment has been minimized.

Sign in to view
Jacobfaib
Jacobfaib approved these changes Jul 7, 2026
View reviewed changes
Comment thread libcudacxx/include/cuda/__launch/configuration.h Outdated
if (__n * sizeof(value_type) > __max_portable_dyn_smem_size)
using value_type = typename dynamic_shared_memory_option<_Tp>::value_type;
const auto [__dyn_smem_size, __overflow] = ::cuda::mul_overflow(__n, sizeof(value_type));
if (__dyn_smem_size > __max_portable_dyn_smem_size || __overflow)

@Jacobfaib Jacobfaib Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would swap these conditions and check __overflow first.

Given the nature of overflow I am afraid that an overzealous compiler could interpret __dyn_smem_size > __max_portable_dyn_smem_size (which would be false on overflow) to mean that __overflow could not possibly be true, and therefore delete the check.

I.e. I am envisioning a scenario similar to deleted nullptr checks:

int *ptr = ...;
int x = *ptr;

if (!ptr) {
  // This entire branch is deleted because you deref ptr above
  // and because dereferencing a nullptr is UB, the compiler
  // is allowed to assume it cannot happen.
}

@davebayer davebayer Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mul_overflow always has well defined output, so that shouldn't ever happen. But yeah, why not

@davebayer davebayer force-pushed the fix_overflow_dynamic_smem branch from a6109c1 to 3c29a94 Compare July 8, 2026 06:47
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

😬 CI Workflow Results

🟥 Finished in 1h 39m: Pass: 97%/120 | Total: 1d 08h | Max: 1h 04m | Hits: 91%/385359

See results here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pciolkosz pciolkosz pciolkosz approved these changes
@Jacobfaib Jacobfaib Jacobfaib approved these changes

Assignees

No one assigned

Labels

backport branch/3.4.x

Projects

CCCL
Status: In Review

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@davebayer @pciolkosz @Jacobfaib